Skip to content

WIP: Add Testing Failcase for DSN Parsing #1159

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 4 commits into from
Closed

WIP: Add Testing Failcase for DSN Parsing #1159

wants to merge 4 commits into from

Conversation

ranchodeluxe
Copy link

@ranchodeluxe ranchodeluxe commented Jun 7, 2024 •
edited
Loading

Problem

Crunchydata Postgresql Operator creates passwords by default using the US-ASCII spec. Some of them with special characters in it including @, [, ], / that seem to cause problems for asyncpg host parsing even after running urllib.parse.quote on the password.

The error below shows it return the password assuming it's the port from an example app:

  File "/usr/local/lib/python3.8/site-packages/asyncpg/connect_utils.py", line 633, in _parse_connect_arguments
    addrs, params = _parse_connect_dsn_and_args(
  File "/usr/local/lib/python3.8/site-packages/asyncpg/connect_utils.py", line 288, in _parse_connect_dsn_and_args
    host, port = _parse_hostlist(dsn_hostspec, port, unquote=True)
  File "/usr/local/lib/python3.8/site-packages/asyncpg/connect_utils.py", line 229, in _parse_hostlist
    hostlist_ports.append(int(hostspec_port))
ValueError: invalid literal for int() with base 10: 'a2Vw:yk=)CdSis[fek]tW='

Test output (from this PR) running locally:

――――――――――――――――――――――――――――――――――――――――――――――――― TestConnectParams.test_connect_params ―――――――――――――――――――――――――――――――――――――――――――――――――
Traceback (most recent call last):
  File "/usr/local/Cellar/[email protected]/3.11.8/Frameworks/Python.framework/Versions/3.11/lib/python3.11/unittest/case.py", line 57, in testPartExecutor
    yield
  File "/usr/local/Cellar/[email protected]/3.11.8/Frameworks/Python.framework/Versions/3.11/lib/python3.11/unittest/case.py", line 623, in run
    self._callTestMethod(testMethod)
  File "/usr/local/Cellar/[email protected]/3.11.8/Frameworks/Python.framework/Versions/3.11/lib/python3.11/unittest/case.py", line 579, in _callTestMethod
    if method() is not None:
       ^^^^^^^^
  File "/Users/ranchodeluxe/apps/asyncpg/tests/test_connect.py", line 1186, in test_connect_params
    self.run_testcase(testcase)
  File "/Users/ranchodeluxe/apps/asyncpg/tests/test_connect.py", line 1106, in run_testcase
    addrs, params = connect_utils._parse_connect_dsn_and_args(
                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/ranchodeluxe/apps/asyncpg/asyncpg/connect_utils.py", line 296, in _parse_connect_dsn_and_args
    host, port = _parse_hostlist(dsn_hostspec, port, unquote=True)
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/ranchodeluxe/apps/asyncpg/asyncpg/connect_utils.py", line 231, in _parse_hostlist
    hostlist_ports.append(int(hostspec_port))
                          ^^^^^^^^^^^^^^^^^^
ValueError: invalid literal for int() with base 10: 'a2Vw:yk=)CdSis[fek]tW='

@ranchodeluxe
Copy link
Author

ranchodeluxe commented Jun 7, 2024 •
edited
Loading

to get this test passing locally using the testing infrastructure I had to:

  1. remove these problematic character/ then urllib.parse.quote worked:

revised passing test

        {
            'name': 'dsn_bad_characters_maybe',
            'dsn': 'postgres://eoapi:a2Vw%3Ayk%3D%29CdSis%5Bfek%5DtW%[email protected]:5432/eoapi',
            'result': ([('eoapi-primary.default.svc', 5432)], {
                'user': 'eoapi',
                'password': 'a2Vw:yk=)CdSis[fek]tW=',
                'database': 'eoapi',
                'ssl': True,
                'sslmode': SSLMode.prefer
                ,
                'target_session_attrs': 'any'})
        },

@ranchodeluxe ranchodeluxe mentioned this pull request Jun 7, 2024
Closed
@ranchodeluxe ranchodeluxe changed the title WIP: special characters in password throw off parsing credentials Add Testing Failcase for DSN Parsing Jun 7, 2024
@ranchodeluxe
Copy link
Author

I see we have a _validate_port_spec.

If passwords don't allow certain characters should we validate and raise too?

@ranchodeluxe ranchodeluxe changed the title Add Testing Failcase for DSN Parsing WIP:Add Testing Failcase for DSN Parsing Jun 7, 2024
@ranchodeluxe ranchodeluxe changed the title WIP:Add Testing Failcase for DSN Parsing WIP: Add Testing Failcase for DSN Parsing Jun 7, 2024
@ranchodeluxe
Copy link
Author

ranchodeluxe commented Jun 7, 2024 •
edited
Loading

thanks @elprans : urllib.parse.quote_plus() works, test is up to date

will close this

$ pytest tests/test_connect.py -k TestConnectParams

 tests/test_connect.py ✓✓✓✓✓✓✓✓✓✓                                                                                         100% ██████████

Results (0.09s):
      10 passed
      41 deselected

ranchodeluxe added a commit to ranchodeluxe/stac-fastapi-pgstac that referenced this pull request Jun 7, 2024
@ranchodeluxe
use quote_plus instead of quote
e2fe26f 
`asyncpg` doesn't work well for all US-ASCII characters, we need to use `quote_plus` instead

MagicStack/asyncpg#1159
@ranchodeluxe ranchodeluxe mentioned this pull request Jun 7, 2024
Merged
4 tasks
vincentsarago added a commit to stac-utils/stac-fastapi-pgstac that referenced this pull request Jun 27, 2024
@ranchodeluxe @vincentsarago
use quote_plus instead of quote (#122)
8b61a87 
* use quote_plus instead of quote

`asyncpg` doesn't work well for all US-ASCII characters, we need to use `quote_plus` instead

MagicStack/asyncpg#1159

* make it fail

* fix test

* one more quote

---------

Co-authored-by: ranchodeluxe <[email protected]>
Co-authored-by: vincentsarago <[email protected]>
@elprans elprans closed this Oct 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ranchodeluxe @elprans